I don't think this will work? This is for a parameter declared as rule<T>, which at the Rust level is a closure returning RuleResult<T>. To work properly, the cache key would need to be the identity of the closure definition site plus any closed-over upvars, which would be quite difficult to do. So this should probably remain forbidden.

You’re right, I improperly assumed that prohibiting generic types was sufficient for this case because I almost never used rule with a concrete type. Now that you point it out, it is embarrassingly obvious that RuleParamTy appearing here actually means I needed to think about it more :-)

Besides lifetimes, this is going to break for things like &str. I don't think trying to turn a borrowed type into an owned type syntactically is going to be robust enough, even using syn.

The right way would be to use traits to get rustc to do it for you in the type system. ToOwned is close, but is implemented for T rather than &T. It might be hard to handle that because &T implements Clone so there couldn't be a T: Clone blanket impl and a separate one for references.

The simplest and most consistent way would be to just require Clone + Hash + 'static. If you're ok with this cloning the argument implicitly to use as the cache key, it seems like it should also be fine to pass by value.

What you are saying makes sense.

I approached this largely pragmatically where a move away from “you can’t do this at all” was a step in the right direction, rather than doing the absolute best engineering. (It feels bad to say that when contributing to someone else’s project…) So I considered it OK to just have a compilation error in situations like what you describe with &str, though you make a great point that using <T as ToOwned>::Owned makes more sense—I always forget about this. (AFAIK it would still require stripping the reference in a hacky way like this, though?)

The issue of value versus reference types in cases similar to this is one which has come up in another crate I maintain and I never really came up with a good solution for what to do in the absence of type metaprogramming, and so I am not sure if there is any general solution for this case either.

I think the main issue with requiring cacheable arguments to be passed by value is that cloning is only necessary when inserting to the cache. For rules where there is a cache hit, this would just be wasted work. Additionally, in my case, requiring to pass by value would also require adding Rc, whereas right now it is not needed. These are not dealbreakers but it does feel bad to artificially restrict performance in a feature intended to improve performance.

Oh, I missed the part where you mentioned interior mutability 😱. In that case, Rc doesn't even save you because then you'd be mutating the copy used as the cache HashMap key, which it's not going to like.

I'm kind of inclined to say that rule arguments should be simple, immutable, maybe even Copy types and anything beyond that is asking for trouble. For example, I think pub rules already require arguments to be Copy when they re-parse for error position.

Would it work for your use case to have an escape hatch attribute, maybe #[cache_ignore] that excludes an argument from the cache key but passes it into the rule anyway? You would pass your context twice, once immutable by value for the cache key, and once as a mutable reference and #[cache_ignored] for the rule to mutate.

In that case, Rc doesn't even save you because then you'd be mutating the copy used as the cache HashMap key, which it's not going to like.

Since there are no blanket implementations of Hash for interior-mutable types, I have to implement that trait myself, so I think this is no issue, as the Rc implementation is just a pass-through, and it really would just be the performance loss?

Would it work for your use case to have an escape hatch attribute, maybe #[cache_ignore] that excludes an argument from the cache key but passes it into the rule anyway?

I am not sure, I think I would have to run some tests, or wait for a day when I can think better :-). The context would actually be Copy except for these damnable interior-mutable properties :-).

To explain more what is going on here in the first place: The original Wikitext parser did not backtrack for mismatched pairs; once it saw [[ {{ it would only ever look for }}. This means the obvious rule for parsing templates…

rule template()
  = "{{" t:template_content() "}}" { Token::Template(t) }
  / "{{" { Token::Text }

…does not work, because for the input "[[ {{ ]]" it would produce Link { content: Text("{{") } while the “correct” production is actually Text("[[ {{ ]]").

To address this, whenever a mismatched pair is encountered, the fallback subexpression (rule broken_<whatever>) consumes the opening delimiter and then clears ctx.prod_kind. This causes whichever terminator had been set by the parent rule to no longer match in inline_breaks (the rule which decides whether an expression is finished), which causes the child to consume it instead of stopping at the terminator, which makes the parent rule fail and match its fallback subexpression, and on and on, until all the previous unclosed delimiters are consumed as text.

So, again using the example "[[ {{ ]]", the order of operation becomes:

And the rule looks (approximately) like:

rule template(ctx: &Context)
  = t:(
      ctx:({ ctx.with_prod_kind(Some(ProdKind::Template)) })
      "{{" t:template_content(&ctx) inline_breaks(&ctx) "}}" { Token::Template(t) }
    / /* broken_template */ "{{" { ctx.prod_kind.set(None); Token::Text }
  )
  { t }

It would be great to find another way to handle this but this is what the upstream developers came up with in their own PEG which I have adapted.

The simplest and most consistent way would be to just require Clone + Hash + 'static.

It seems to me, that most idiomatic way is to introduce Cacheable trait, implement it for types, that you known is safe to cache (maybe via blanket impl), and cache them using methods on that trait.